-
Notifications
You must be signed in to change notification settings - Fork 15k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: EventEmitter is missing properties in sandbox preload script. #35522
Conversation
Beside added test here is a gist to test it : https://gist.github.com/eec01457cf39fb999b6e6ce775912476 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please expand the release notes section in the PR description!
@@ -45,6 +45,7 @@ | |||
"eslint-plugin-node": "^11.1.0", | |||
"eslint-plugin-standard": "^4.0.1", | |||
"eslint-plugin-typescript": "^0.14.0", | |||
"events": "^3.2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does webpack automatically detect this module somehow? I'm surprised that just adding it to package.json is enough to have this be added to the preload bundle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for late answer, I was on vacation. I have not looked into webpack internals that much to be honest. Most migration guides state that you should include what you use, so I just did that, similarly to original PR adding process and buffer.
spec/api-browser-window-spec.ts
Outdated
@@ -2970,6 +2970,26 @@ describe('BrowserWindow module', () => { | |||
expect(url).to.equal(expectedUrl); | |||
}); | |||
|
|||
it('exposes full EventEmmiter object to preload script', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it('exposes full EventEmmiter object to preload script', async () => { | |
it('exposes full EventEmitter object to preload script', async () => { |
let rendererEventEmitterProperties = ''; | ||
let currentObj = emitter; | ||
do { | ||
Object.getOwnPropertyNames(currentObj).map(property => { rendererEventEmitterProperties += property + ';'; }); | ||
} while ((currentObj = Object.getPrototypeOf(currentObj))); | ||
const { ipcRenderer } = require('electron'); | ||
ipcRenderer.send('answer', rendererEventEmitterProperties); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be an array of strings, no need to flatten it out to a string.
let rendererEventEmitterProperties = ''; | |
let currentObj = emitter; | |
do { | |
Object.getOwnPropertyNames(currentObj).map(property => { rendererEventEmitterProperties += property + ';'; }); | |
} while ((currentObj = Object.getPrototypeOf(currentObj))); | |
const { ipcRenderer } = require('electron'); | |
ipcRenderer.send('answer', rendererEventEmitterProperties); | |
let rendererEventEmitterProperties = []; | |
let currentObj = emitter; | |
do { | |
rendererEventEmitterProperties.push(...Object.getOwnPropertyNames(currentObj)); | |
} while ((currentObj = Object.getPrototypeOf(currentObj))); | |
const { ipcRenderer } = require('electron'); | |
ipcRenderer.send('answer', rendererEventEmitterProperties); |
Release Notes Persisted
|
Description of Change
Add EventEmitter to package.json (version is based on webpack 5 requirements).
After update to webpack 5 missing events dependency was causing EventEmitter created in renderer preload script to be missing properties, for example eventNames, getMaxListeners, prependListener, prependOnceListener, etc. Add it so that it can be used properly from electron libs, sandbox renderer init scripts specifically.
Checklist
npm test
passesRelease Notes
Notes: Fixed an issue with incomplete EventEmitter object in sandboxed renderer preload script.